Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Respect SETTINGS_HEADER_TABLE_SIZE http2 setting #2045

Merged
merged 4 commits into from
May 11, 2018
Merged

Respect SETTINGS_HEADER_TABLE_SIZE http2 setting #2045

merged 4 commits into from
May 11, 2018

Conversation

lyuxuan
Copy link
Contributor

@lyuxuan lyuxuan commented May 2, 2018 •
edited
Loading

fix #1928

@dfawley dfawley requested a review from MakMukhi May 3, 2018 20:39
This was referenced May 9, 2018
Closed
Closed
@mumoshu
Copy link

mumoshu commented May 10, 2018

@lyuxuan This worked like a charm! nginx-ingress-controller is now happy to load-balance grpc over h2c workloads to the grpc server w/ this change. Thanks a lot for the fix 👍

MakMukhi
MakMukhi suggested changes May 11, 2018
View reviewed changes
transport/transport_test.go Outdated

var svrTransport ServerTransport
var i int
for i := 0; i < 1000; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This i is not same as the i above. := assigns a new local variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

transport/transport_test.go
}
break
}
if i == 1000 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing i is local to the for loop above.

@MakMukhi MakMukhi assigned lyuxuan and unassigned MakMukhi May 11, 2018
@lyuxuan lyuxuan merged commit 9a54b9a into grpc:master May 11, 2018
@lyuxuan lyuxuan added the Type: Feature New features or improvements in behavior label May 11, 2018
@xjewer xjewer mentioned this pull request Jun 7, 2018
Merged
ghost pushed a commit to hyperledger/fabric that referenced this pull request Jul 3, 2018
@mastersingh24
[FAB-10885] Upgrade grpc-go
83f18e7 
We should upgrade anyway, but
grpc/grpc-go#2045
prevents the use of proxies such as nginx
from working with Fabric nodes.

Change-Id: I3dc8af85ede23548f26c8fc38f984b94f6cc4e4b
Signed-off-by: Gari Singh <gari.r.singh@gmail.com>
@him0
Copy link

him0 commented Sep 12, 2018

I cannot find this PR at release note , but found this changes in version 1.15.0(8dea3dc). Has this pull request already released?

grpc-go/internal/transport/controlbuf.go

Lines 31 to 33 in 8dea3dc

var updateHeaderTblSize = func(e *hpack.Encoder, v uint32) {
e.SetMaxDynamicTableSizeLimit(v)
}

@lyuxuan
Copy link
Contributor Author

lyuxuan commented Sep 12, 2018

@him0, this feature is released since version 1.13.0. You can look at the commit page here, and figure out the versions having this feature. We forgot to include it in the 1.13.0 release note, which is an unfortunate oversight. Thanks!

@nwest1 nwest1 mentioned this pull request Oct 23, 2018
Closed
benley added a commit to benley/jaeger that referenced this pull request Feb 8, 2019
@benley
Require a slightly newer version of grpc-go
2980815 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538
benley added a commit to benley/jaeger that referenced this pull request Feb 8, 2019
@benley
Require a slightly newer version of grpc-go
fd6d99c 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538
@benley benley mentioned this pull request Feb 8, 2019
Merged
benley added a commit to benley/jaeger that referenced this pull request Feb 8, 2019
@benley
Require a slightly newer version of grpc-go
6699888 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
benley added a commit to postmates/jaeger that referenced this pull request Feb 8, 2019
@benley
Require a slightly newer version of grpc-go
aa3c9df 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538
yurishkuro pushed a commit to jaegertracing/jaeger that referenced this pull request Feb 8, 2019
@benley @yurishkuro
Require a slightly newer version of grpc-go (#1324)
c4bb6b5 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
@benley @iori-yja
Require a slightly newer version of grpc-go (jaegertracing#1324)
d9181f3 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
Signed-off-by: Iori Yoneji <fivo.11235813@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
@benley @iori-yja
Require a slightly newer version of grpc-go (jaegertracing#1324)
0e12504 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
@benley @iori-yja
Require a slightly newer version of grpc-go (jaegertracing#1324)
728c8d9 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
iori-yja pushed a commit to iori-yja/jaeger that referenced this pull request Feb 15, 2019
@benley @iori-yja
Require a slightly newer version of grpc-go (jaegertracing#1324)
9e44e3c 
This includes the fix for grpc/grpc-go#1928
(grpc/grpc-go#2045), which makes
jaeger-collector usable behind nginx (and probably other grpc/http2
implementations) as load balancers for its grpc endpoint.

More info: https://trac.nginx.org/nginx/ticket/1538

Signed-off-by: Benjamin Staffin <benley@gmail.com>
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mumoshu mumoshu mumoshu approved these changes

@MakMukhi MakMukhi MakMukhi approved these changes

Assignees

@lyuxuan lyuxuan

Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Respect SETTINGS_HEADER_TABLE_SIZE http2 setting
4 participants
@lyuxuan @mumoshu @him0 @MakMukhi